Skip to content

feat(history-service): return minUserLastSeenAt from LoadHistory RPC#161

Merged
mliu33 merged 11 commits intomainfrom
claude/add-minuserlastsenat-loadhistory-6rT4X
May 8, 2026
Merged

feat(history-service): return minUserLastSeenAt from LoadHistory RPC#161
mliu33 merged 11 commits intomainfrom
claude/add-minuserlastsenat-loadhistory-6rT4X

Conversation

@Allan-Code-hub
Copy link
Copy Markdown
Collaborator

@Allan-Code-hub Allan-Code-hub commented May 7, 2026

Summary

Add minUserLastSeenAt field to the LoadHistory NATS RPC response, sourced from the MongoDB rooms collection. This allows clients to receive the per-room read floor alongside messages in a single round-trip, eliminating the need for a separate request to fetch room-level read-state metadata.

Key Changes

  • New RoomRepo in mongorepo: Thin repository layer that reads room.minUserLastSeenAt via a PK-projected findOne query. Returns (nil, nil) for missing documents or unset fields, treating both as "no read floor".

  • RoomRepository interface: Added to service.go as a consumer interface, injected into HistoryService. Includes go:generate directive update to regenerate mocks.

  • LoadHistory handler enhancement: Now reads the room floor in parallel with the Cassandra message page fetch using errgroup. Errors are logged at Warn level and the response field is omitted (graceful degradation). The field is encoded as UTC milliseconds and uses omitempty to exclude it from JSON when nil.

  • Response model update: LoadHistoryResponse now includes MinUserLastSeenAt *int64 field with omitempty tag.

  • Wiring in cmd/main.go: RoomRepo is instantiated and injected into service.New().

  • Test helper refactoring: Split newService into two helpers—newService (with permissive room mock default for backward compatibility) and newServiceWithRoomMock (for tests that need to set specific room expectations). Added three new test cases for the LoadHistory field and two negative-coupling guards on sibling RPCs (LoadNextMessages, LoadSurroundingMessages) to prevent accidental future regressions.

  • Documentation: Updated docs/client-api.md to document the new response field and provided example payload.

Implementation Details

  • Parallel I/O: The Cassandra page read and MongoDB room read execute concurrently using errgroup.WithContext, improving latency.
  • Error handling: Room read errors are non-fatal; they log a warning and the response field is omitted. Message loading continues unaffected.
  • No schema changes: Uses existing rooms collection; no new indexes required (PK lookup only).
  • Backward compatible: Existing tests pass without modification due to the permissive default expectation on the room mock.
  • Compile-time safety: Added var _ check to ensure *mongorepo.RoomRepo satisfies RoomRepository interface.

Testing

  • Integration tests for RoomRepo covering set, unset, and missing document cases.
  • Unit tests for LoadHistory covering successful read, nil floor, and error degradation.
  • Negative-coupling guards on sibling RPCs to prevent accidental room reads.
  • All existing tests pass without modification.

https://claude.ai/code/session_01RbwBSYChUkREPjFfJjPADg

Summary by CodeRabbit

Release Notes

  • New Features

    • LoadHistory response now includes an optional minUserLastSeenAt field, indicating the minimum message read timestamp across room members. This eliminates the need for additional client queries to determine room-level read state.
  • Documentation

    • Updated API documentation with the new minUserLastSeenAt field and example response payloads.
  • Tests

    • Added comprehensive integration and unit tests validating the new read-floor tracking functionality and graceful degradation on errors.

claude added 10 commits May 7, 2026 06:39
Spec for adding the room read floor to the LoadHistory NATS RPC response,
sourced via a new history-service RoomRepo doing a PK-projected findOne on
the rooms collection. Encoded as UTC millis with omitempty; degrades to
log-and-omit on read errors.

https://claude.ai/code/session_01RbwBSYChUkREPjFfJjPADg
Seven-task TDD plan covering the new RoomRepo, RoomRepository interface +
mock regeneration, HistoryService wiring (with a permissive default on the
existing test helper to keep ~100 call sites untouched), the response model
+ handler change, sibling-RPC negative-coupling guards, client-api docs,
and final lint/test/integration/coverage verification.

https://claude.ai/code/session_01RbwBSYChUkREPjFfJjPADg
…y arg

The constructor signature gained a RoomRepository parameter; the three
integration test call sites need a stub that satisfies the interface.
nilRoomRepo returns (nil, nil) — these tests don't exercise the room read.
- RoomRepo now uses Collection[model.Room] + WithProjection to match
  SubscriptionRepo, fixing a bare-error return and dropping the duplicate
  inline anonymous struct.
- LoadHistory parallelizes the Cassandra page read and the rooms.findOne
  via errgroup, removing one Mongo RTT from the hot path.
- Strip a few narrative comments per project guidance.
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 7, 2026

Review Change Stack

Warning

Rate limit exceeded

@Allan-Code-hub has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 48 minutes and 20 seconds before requesting another review.

To continue reviewing without waiting, purchase usage credits in the billing tab.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: d49058ae-5448-4ad1-a4f5-cbafa7f57e3a

📥 Commits

Reviewing files that changed from the base of the PR and between 5d1744f and 639fee2.

📒 Files selected for processing (2)
  • docs/superpowers/specs/2026-05-07-loadhistory-min-user-last-seen-at-design.md
  • history-service/internal/mongorepo/room.go
📝 Walkthrough

Walkthrough

This PR introduces a minUserLastSeenAt optional field to the LoadHistory RPC response by adding MongoDB repository abstraction for rooms collection queries, wiring it through HistoryService, and fetching the value concurrently in the LoadHistory handler. The field is best-effort; Mongo read errors are logged without failing the response.

Changes

LoadHistory Room Read Floor Feature

Layer / File(s) Summary
Specification & Design
docs/superpowers/specs/2026-05-07-loadhistory-min-user-last-seen-at-design.md, docs/superpowers/plans/2026-05-07-loadhistory-min-user-last-seen-at.md, docs/client-api.md
Design specs define wire contract (optional int64 UTC millis with omitempty), repository abstraction (RoomRepository interface + RoomRepo Mongo implementation), service wiring (HistoryService constructor parameter), handler changes (concurrent fetch with error logging), and test strategy (presence, absence JSON omission, degradation, sibling-RPC guards). Client API docs updated with new response field and semantics.
Data Model
history-service/internal/models/message.go
LoadHistoryResponse extended with MinUserLastSeenAt *int64 field (UTC millis, omitted when nil).
Repository Abstraction
history-service/internal/service/service.go, history-service/internal/mongorepo/room.go
New RoomRepository interface with GetMinUserLastSeenAt(ctx, roomID) method. RoomRepo MongoDB implementation performs projected findOne on rooms collection, returns (nil, nil) when document/field missing, propagates query errors.
Service Wiring
history-service/internal/service/service.go
HistoryService struct extended with rooms RoomRepository field; New constructor signature updated to accept RoomRepository parameter; mockgen and compile-time assertions include RoomRepository.
Main Initialization
history-service/cmd/main.go
Creates roomRepo via mongorepo.NewRoomRepo(db) and passes into service.New(...) constructor.
Handler Implementation
history-service/internal/service/messages.go
LoadHistory refactored to fetch message page and room minUserLastSeenAt concurrently using errgroup; non-fatal Mongo errors logged; MinUserLastSeenAt conditionally included in response when available (converted to UTC millis).
Mock Generation
history-service/internal/service/mocks/mock_repository.go
Auto-generated MockRoomRepository with mocked GetMinUserLastSeenAt method for unit tests.
Unit Tests
history-service/internal/service/messages_test.go
Test helpers refactored to provide MockRoomRepository; new LoadHistory tests verify minUserLastSeenAt populated, omitted when absent (JSON omitempty), and gracefully degraded on errors.
Integration Tests
history-service/internal/mongorepo/room_test.go, history-service/internal/service/integration_test.go
RoomRepo integration tests verify behavior with set/unset/missing minUserLastSeenAt; service integration tests stubbed with nilRoomRepo to guard against sibling RPCs reading rooms.

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

  • hmchangw/chat#143: Introduces Room.MinUserLastSeenAt in shared model; this PR wires that field into history-service repository, handler, and response.
  • hmchangw/chat#156: Implements writing/updating rooms.minUserLastSeenAt in room-service; this PR implements concurrent reading of the same field in history-service.
  • hmchangw/chat#144: Both modify history-service wiring (service.New, dependencies), add repository interfaces/mocks, and update cmd/main.go initialization.

Suggested reviewers

  • mliu33

Poem

🐰 A floor for the room, now swift as the breeze,
Concurrent it fetches, with ease and with grace,
Mongo holds secrets, read once at a pace,
LoadHistory flows on—no roundtrips, just please! ✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 23.81% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: adding a new minUserLastSeenAt field to the LoadHistory RPC response in the history-service.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch claude/add-minuserlastsenat-loadhistory-6rT4X

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (1)
history-service/internal/service/service.go (1)

47-49: 🏗️ Heavy lift

Rename single-method interface to match repository naming rule.

At Line 47, RoomRepository is a single-method interface; per project convention, this should use an -er suffix (e.g., RoomReader) or the <Domain>Store form (e.g., RoomStore).

As per coding guidelines, "Interface names must use -er suffix for single-method interfaces or <Domain>Store pattern for store interfaces".

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@history-service/internal/service/service.go` around lines 47 - 49, The
interface named RoomRepository is a single-method interface
(GetMinUserLastSeenAt) and should be renamed to follow the project convention;
change the interface name to a suffix-`-er` form (e.g., RoomReader) or a store
form (e.g., RoomStore) and update all references/usages accordingly (constructor
params, variable declarations, mock implementations, and any function signatures
that accept RoomRepository) so that GetMinUserLastSeenAt remains the method on
the newly named interface.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In
`@docs/superpowers/specs/2026-05-07-loadhistory-min-user-last-seen-at-design.md`:
- Around line 20-21: The doc currently states the room read is sequential but
the implementation runs both the new Mongo read and the existing Cassandra page
read in parallel using errgroup; update the spec text describing the
handler/room read to state it executes the Mongo and Cassandra reads
concurrently (using errgroup or equivalent concurrency primitive), note how
errors/aggregation are handled, and adjust any language in the other room-read
paragraphs that imply serial execution so they match the implemented handler
execution model.

In `@history-service/internal/mongorepo/room.go`:
- Around line 32-34: Replace the bare `return nil, err` in room.go with a
wrapped error that describes the operation and includes the original error
(e.g., `fmt.Errorf("failed to fetch room %s: %w", roomID, err)` or
`fmt.Errorf("failed to query room with filter %v: %w", filter, err)`), import
fmt if needed, and use the actual identifier used in the function (roomID or
filter) so callers get contextual information about which room/query failed.

---

Nitpick comments:
In `@history-service/internal/service/service.go`:
- Around line 47-49: The interface named RoomRepository is a single-method
interface (GetMinUserLastSeenAt) and should be renamed to follow the project
convention; change the interface name to a suffix-`-er` form (e.g., RoomReader)
or a store form (e.g., RoomStore) and update all references/usages accordingly
(constructor params, variable declarations, mock implementations, and any
function signatures that accept RoomRepository) so that GetMinUserLastSeenAt
remains the method on the newly named interface.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 55b55c5c-864c-4948-800e-31b5b318cdaf

📥 Commits

Reviewing files that changed from the base of the PR and between 61f128a and 5d1744f.

📒 Files selected for processing (12)
  • docs/client-api.md
  • docs/superpowers/plans/2026-05-07-loadhistory-min-user-last-seen-at.md
  • docs/superpowers/specs/2026-05-07-loadhistory-min-user-last-seen-at-design.md
  • history-service/cmd/main.go
  • history-service/internal/models/message.go
  • history-service/internal/mongorepo/room.go
  • history-service/internal/mongorepo/room_test.go
  • history-service/internal/service/integration_test.go
  • history-service/internal/service/messages.go
  • history-service/internal/service/messages_test.go
  • history-service/internal/service/mocks/mock_repository.go
  • history-service/internal/service/service.go

Comment thread docs/superpowers/specs/2026-05-07-loadhistory-min-user-last-seen-at-design.md Outdated
Comment thread history-service/internal/mongorepo/room.go
- spec: rewrite the LoadHistory handler section to describe the actual
  errgroup-based concurrent execution (room read in parallel with the
  Cassandra page read), drop the now-stale sequential prose and the
  "parallelize as a follow-up" non-goal/follow-up bullets.
- mongorepo/room.go: wrap the bare error return with context describing
  the operation and the roomID, per the project's error-handling guideline.
@Allan-Code-hub Allan-Code-hub requested a review from mliu33 May 7, 2026 09:55
Copy link
Copy Markdown
Collaborator

@mliu33 mliu33 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work, thanks!

@mliu33 mliu33 merged commit 1807b75 into main May 8, 2026
5 checks passed
vjauhari-work pushed a commit that referenced this pull request May 8, 2026
PR #161 (1807b75) introduced room.go and a usage of mongorepo.RoomRepo
in service.go but missed:
- Adding the pkg/mongoutil import in room.go (Collection, NewCollection,
  WithProjection are unqualified there, which compiles only with stricter
  go vet on newer golangci-lint).
- Adding the mongorepo import in service.go for the var _ assertion.

Lint v2.11.4 (CI version) catches both as typecheck failures; older lint
versions silently passed. Apply the import fixes so the rebased PR #164
passes CI.

Includes a rebase onto latest main.
vjauhari-work pushed a commit that referenced this pull request May 8, 2026
PR #161 (1807b75) introduced room.go and a usage of mongorepo.RoomRepo
in service.go but missed:
- Adding the pkg/mongoutil import in room.go (Collection, NewCollection,
  WithProjection are unqualified there, which compiles only with stricter
  go vet on newer golangci-lint).
- Adding the mongorepo import in service.go for the var _ assertion.

Lint v2.11.4 (CI version) catches both as typecheck failures; older lint
versions silently passed. Apply the import fixes so the rebased PR #164
passes CI.

Includes a rebase onto latest main.
vjauhari-work added a commit that referenced this pull request May 8, 2026
…HistoryConfig.SharedSince (#164)

* docs(sync-dm): spec for sync server-to-server DM endpoint + HistoryConfig.SharedSince removal

Two related-but-distinct cleanups in a single design document:

Part 1 — Sync server-to-server DM creation. New NATS request/reply
endpoint chat.server.request.room.{siteID}.create.dm hosted by
room-worker (server credentials, queue group room-worker). The endpoint
is intentionally persistence-only:

  - Caller-side validation contract: user-service performs all
    data-integrity checks (existence, EngName/ChineseName, bot
    Assistant.Enabled, self-DM, dedup-existing via FindDMSubscription)
    before issuing the request.
  - room-worker performs only request-shape sanity checks, resolves
    requester/other User records via GetUser for data (User.ID,
    User.SiteID), persists Room + 2 subs, emits subscription.update
    events and cross-site room_created outbox event, and replies with
    the requester's Subscription.
  - Idempotent against retries and concurrent races via duplicate-key
    fallback at insert time.

Request shape (3 fields):
  type SyncCreateDMRequest struct {
      RoomType         RoomType `json:"roomType"`
      RequesterAccount string   `json:"requesterAccount"`
      OtherAccount     string   `json:"otherAccount"`
  }

Reply shape:
  type SyncCreateDMReply struct {
      Success      bool               `json:"success"`
      Subscription model.Subscription `json:"subscription"`
  }

Errors via standard natsutil.ReplyError -> model.ErrorResponse. No new
shared package; small internal buildDMSubs / buildBotDMSubs extractions
inside room-worker. room-service is not touched on this path.

Part 2 — Remove HistoryConfig.SharedSince. HistoryConfig now carries
only Mode. Subscription.HistorySharedSince is derived exclusively from
acceptedAt (the request-acceptance time stamped by room-service) when
HistoryMode == "none". historySharedSincePtr signature reduces to
(mode HistoryMode, acceptedAt time.Time) *int64.

https://claude.ai/code/session_015HZFYkTm4gPbZmq2NYRUpb

* docs/plans: add sync DM endpoint Part 1 and Part 2 plans

Part 1 specifies the synchronous DM-create endpoint. Part 2 removes the
unused HistoryConfig.SharedSince override field to simplify the add-member
request schema before downstream code stabilizes around it.

https://claude.ai/code/session_015HZFYkTm4gPbZmq2NYRUpb

* feat(pkg/model): add SyncCreateDMRequest and SyncCreateDMReply types

* feat(pkg/subject): add RoomCreateDMSync builder

* test(pkg/subject): fix RoomCreateDMSync test to use t.Errorf style

* feat(room-worker): add FindDMSubscription to SubscriptionStore

* refactor(room-worker): extract buildDMSubs/buildBotDMSubs helpers

* feat(room-worker): sync DM handler sentinels and sanitizer

* feat(room-worker): handleSyncCreateDM request-shape validation

* feat(room-worker): resolve User records in sync DM handler

* feat(room-worker): persist Room with duplicate-key fallback

* feat(room-worker): persist subs with duplicate-key fallback and reply

* feat(room-worker): publish subscription.update events from sync DM handler

* feat(room-worker): emit cross-site room_created outbox from sync DM handler

* feat(room-worker): wire sync DM NATS endpoint in main

* test(room-worker): integration tests for sync DM endpoint

* room-worker: drop tests for HistoryConfig.SharedSince override

* pkg/model,room-worker: remove HistoryConfig.SharedSince override

* fmt(pkg/minioutil): apply goimports to integration test struct literals

* style: trim verbose comments to single-line summaries

* room-worker: address review findings on sync DM handler

- B1: stop wrapping the errUserLookupFailed sentinel; wrap the real store
  error so transient mongo failures aren't masked as "user not found".
- B2: always re-read the canonical persisted sub via FindDMSubscription
  after BulkCreateSubscriptions. The store swallows duplicate-key races,
  so the in-memory sub may carry stale fields on retry.
- F1: bubble outbox publish errors to the caller. Federation can't recover
  silently — if the cross-site room_created emit fails, the request must
  fail so the caller can retry.
- F2: use the existing outboxDedupID helper for consistency with the
  async outbox call sites.

* room-worker: address review quality items

- Q4: mask errRoomIDCollision as "internal error" on the wire; log details
  server-side. Implementation detail no longer leaks to callers.
- Q5: add bson tags to SyncCreateDMRequest/SyncCreateDMReply per project rule.
- Q9: log natsServerCreateDM errors structurally with subject + requestID
  so operators can diagnose without scraping reply errors.
- Q6: rename test variable cap → capture (avoid shadowing builtin).
- Q7: extract newSyncDMTestHandler helper, mirroring newAddMembersTestHandler.

Q1+Q2 (share helpers/error types between sync and async) deferred —
async uses errPermanent/sanitizeAsyncJobError for JetStream Ack/Nak; sync
uses named sentinels for stable RPC error codes. Different contracts on
purpose; merging would worsen one path. Async path code untouched.

Q8 (drop dm-prefix on test types) deferred — collision with the type of
the same name in integration_test.go. Rename in unit-test file alone
isn't useful; renaming both would touch async-path tests.

Q3 (split handleSyncCreateDM) deferred — ~80 lines doing one logical op,
already covered by per-step tests; split would just shuffle parameters.

* room-worker: merge sync DM feature into handler.go and improve tests

- Merge handler_sync_create_dm.go into handler.go per request to colocate
  the feature with the rest of the handler logic. Production file deleted.
- T3: per-sub field assertions in DM_PersistsSubs test (alice + bob subs
  named correctly, IsSubscribed false, RoomType set).
- T4: table-driven RoomCollisionMismatch covering Type/SiteID/Name/CreatedBy.
- T6: marshalReq helper replaces ignored json.Marshal errors.
- T7: federation-convergence integration test asserts outbox RoomID matches
  the deterministic BuildDMRoomID and that replay produces identical
  Nats-Msg-Id (stream dedup blocks duplicate cross-site emits).
- Q7: extract newSyncDMTestHandler to mirror newAddMembersTestHandler.

T5 (t.Parallel()) deferred — handler_test.go has 0 uses; adding only here
would diverge from existing convention. Q1+Q2 (share helpers/error types
with async path) deferred — different contracts; user instructed no async
refactor.

* fix(room-service): split DM dedup index into two $eq partial indexes

MongoDB's partialFilterExpression rejects $in on most server versions
(only $eq/$exists/$gt/$lt/$type/$and are universally supported).
The single combined index using {roomType: {$in: [dm, botDM]}} fails on
startup with: "unsupported expression in partial index: roomType $in".

Replace with two equivalent indexes — one filtered to {$eq: "dm"}, one
to {$eq: "botDM"} — both with the same (u.account, name, roomType)
unique key. Net effect on the create-room contract is unchanged.

Operators with the broken index from a prior deploy must drop
u_account_name_roomtype_dm_idx manually before redeploying — the new
code creates a fresh index with that name (filtered to $eq dm) and a
new u_account_name_roomtype_botdm_idx.

* room-worker: set DM/botDM Room.UserCount/AppCount at creation

For sync DM creation, we know the membership shape at creation time:
- DM     → UserCount=2, AppCount=0
- botDM  → UserCount=1, AppCount=1

Set these directly on the inserted Room and drop the ReconcileMemberCounts
call on the sync path. Rationale: DMs/botDMs have a fixed roster — there
will never be a future add/remove to trigger a Reconcile pass — so the
spec's "Reconcile failure self-heals on the next membership change"
reasoning doesn't apply. A failed Reconcile would leave the Room with
counts (0, 0) forever; clients would see "0 members" on a 2-person DM.

Setting counts inline removes a Mongo round-trip per call, eliminates
the failure mode, and reduces handler complexity. Async path (channels)
is unchanged — Reconcile is still called via finishCreateRoom because
channels do see future membership changes.

Tests:
- DM unit test asserts inserted Room carries (2, 0).
- botDM unit test asserts (1, 1).
- DM integration test asserts the persisted Mongo Room has (2, 0)
  without ever calling Reconcile.

* room-worker: round-2 cleanup + spec/plan updates

- Drop dead default branch in sync DM room-type switch (validateSyncCreateDMShape
  already gates this upstream).
- Trim two-line FindDMSubscription comment to one line.
- Drop redundant defer ctrl.Finish() (gomock v1.5+ auto-registers cleanup).
- Replace ad-hoc section-divider style with plain section comment to match the file.
- Add unit tests: BulkCreateSubscriptions transient (non-dup-key) error → "internal
  error"; idempotent re-create path asserts sub.JoinedAt = existing.CreatedAt.

Spec + Part 1 plan: add concise table of implementation deviations from the
original design (Reconcile-skip, outbox-fail, always-re-read, error-wrap,
collision-mask, file-merge) plus the room-service partial-index $in→$eq fix.

Items deferred (round 2): roomID-based outbox dedup (would diverge from async
convention which uses requestID); DM AddMember guard (would touch async path);
FindDMSubscription by roomId (async room-service version uses Name).

* room-worker: merge sync DM tests into handler_test.go

Mirrors the production-side merge of handler_sync_create_dm.go into handler.go
done previously. Tests for the sync DM endpoint now live with the rest of the
handler tests; handler_sync_create_dm_test.go deleted.

* address CodeRabbit review comments

- room-service/store_mongo.go: auto-drop the legacy $in-filtered DM dedup
  index in EnsureIndexes if a prior deploy installed it. Eliminates the
  manual operator step CodeRabbit flagged: same name as the new dm index
  but with an incompatible partialFilterExpression would otherwise fail
  CreateMany with IndexOptionsConflict on startup. Idempotent — no-op when
  the index doesn't exist or already has the new $eq spec; logs at warn
  level when it actually drops.
- docs/superpowers/plans/.../part2.md: add `bash` language tag to grep
  block (markdownlint MD040).
- docs/superpowers/specs/...sync-dm-design.md: add `text` language tags
  to topology and handler-pipeline fenced blocks (markdownlint MD040).

* room-worker: use natsutil.ReplyJSON for sync DM reply

Per CodeRabbit review: convention violation — every other NATS reply in
the repo goes through natsutil.ReplyJSON (handles marshal + Respond +
error logging). The sync DM handler was bypassing it with json.Marshal +
m.Msg.Respond directly.

Refactor:
- handleSyncCreateDM now returns (*model.SyncCreateDMReply, error)
  instead of ([]byte, error). Drops the manual marshal block.
- natsServerCreateDM calls natsutil.ReplyJSON(m.Msg, reply); the helper
  handles marshal failure (replies with sanitized error) and respond
  failure (logs).
- Tests no longer json.Unmarshal raw bytes — assert directly on the
  returned struct, simplifying ~10 unit/integration test assertions.

Net: -30 / +5 lines. Same wire format, same observable behavior.

* room-service: revert legacy dm-dedup index auto-drop

The original $in-based partialFilterExpression was rejected by MongoDB at
index-creation time, so no environment ever ended up with a stale legacy
index to clean up — the auto-drop code was dead by construction. Revert
to leave EnsureIndexes minimal; the split-into-two-$eq fix is unchanged.

* fix(history-service): missing mongoutil import broke lint on main

PR #161 (1807b75) introduced room.go and a usage of mongorepo.RoomRepo
in service.go but missed:
- Adding the pkg/mongoutil import in room.go (Collection, NewCollection,
  WithProjection are unqualified there, which compiles only with stricter
  go vet on newer golangci-lint).
- Adding the mongorepo import in service.go for the var _ assertion.

Lint v2.11.4 (CI version) catches both as typecheck failures; older lint
versions silently passed. Apply the import fixes so the rebased PR #164
passes CI.

Includes a rebase onto latest main.

* testimages: pin Mongo to 4.4.15 to match production

Catches operator-allow-list regressions like $in in
partialFilterExpression that newer Mongo silently accepts but production
4.4.x rejects. Same shape as the bug fixed by 2734235 (split DM dedup
index into two $eq partial indexes).

* room-service: drop redundant DM dedup partial indexes

MongoDB 4.4 (production) rejects two indexes with identical key specs
but different partialFilterExpression — IndexOptionsConflict at index
build time. The split-into-two-$eq-indexes fix from 2734235 worked on
Mongo 6+ where partialFilter is part of index identity, but not on 4.4.

The partial-unique constraint was redundant anyway:
- DM rooms use idgen.BuildDMRoomID(a, b), deterministic in user IDs.
  Concurrent same-pair DM creation hits Room._id duplicate-key.
- The existing (roomId, u.account) unique index at line 58 already
  catches duplicate subs per user per room.

Dropping the partial-unique (u.account, name, roomType) indexes restores
Mongo 4.4 compatibility without losing any dedup guarantee.

* room-worker: batch DM user lookup and drop unused publishedMsg.msgID

handleSyncCreateDM did two sequential GetUser round-trips (requester then
other). Collapse to a single FindUsersByAccounts call (per @mliu33's review)
and look up each side from a local map. Cuts one DB round-trip per sync
DM creation.

Also drop publishedMsg.msgID — leftover from the SharedSince test deletion;
golangci-lint flagged it as unused, breaking CI.

https://claude.ai/code/session_01MprNzSB1QQJ7r2GHzRZonY

* room-worker: address sync DM review feedback

Three CodeRabbit review items on PR #164:

1. Publish canonical persisted subs (not in-memory placeholders).
   BulkCreateSubscriptions swallows dup-key races, so on retry the
   in-memory subs may carry IDs/JoinedAt that never made it to Mongo.
   Re-read both subs via FindDMSubscription before fan-out.

2. Stamp event-level Timestamp at publish time. Per project guideline,
   NATS event envelope timestamps must be time.Now().UTC().UnixMilli()
   at publish — not the (potentially stale) acceptedAt. Domain-level
   timestamps (JoinedAt, RoomCreatedOutbox.Timestamp) keep acceptedAt.

3. Spec doc drift: historySharedSincePtr signature in the spec showed
   (mode, acceptedAt) but shipped code kept (history, timestamp, roomID).
   Add a "Shipped deviation" note to the spec.

https://claude.ai/code/session_01MprNzSB1QQJ7r2GHzRZonY

---------

Co-authored-by: Claude <noreply@anthropic.com>
general-lex pushed a commit that referenced this pull request May 8, 2026
PR #161 (1807b75) introduced two undefined-symbol build breaks:
  - mongorepo/room.go used Collection / NewCollection / WithProjection
    without importing pkg/mongoutil (the sibling subscription.go and
    threadroom.go do so correctly with a "mongoutil." prefix).
  - service/service.go's compile-time interface assertion referenced
    mongorepo.RoomRepo without importing the mongorepo package.

Both go build and golangci-lint fail on origin/main HEAD because of
this. Without the fix every PR that merges with main inherits the
break — including this one. Adding the imports + qualifiers locally
unblocks our PR's CI and makes the merge clean.
GITMateuszCharczuk pushed a commit that referenced this pull request May 8, 2026
Pre-existing build break landed in #161 (return minUserLastSeenAt from
LoadHistory RPC). room.go used bare Collection / NewCollection /
WithProjection without importing or qualifying mongoutil, and
service.go referenced *mongorepo.RoomRepo without importing the
package. Repo would not compile or lint, blocking the pre-commit hook.

Surfaced while landing the loadgen scenarios PR; fixing here to unblock.
GITMateuszCharczuk pushed a commit that referenced this pull request May 8, 2026
Small Resource-Owner-Password-Credentials client at pkg/testutil/keycloak.
Used by the upcoming e2e harness to mint JWTs for test users, and reusable
from any future per-service integration test that needs Keycloak-backed
auth without rolling its own HTTP wrapper.

- Client.PasswordGrant / AccessToken / Refresh
- Sentinel errors (ErrInvalidCredentials, ErrUnknownClient, ErrRealmNotFound)
  with errors.Is wired via *Error.Is
- Built on pkg/restyutil so OTel + slog + project-default timeout are inherited
- 97% statement coverage; httptest-based unit tests, no external deps

Spec updated: helper extraction folded into the e2e plan, and the bootstrap
ergonomics now use a checked-in .env.example with a Makefile prerequisite
that auto-creates docker-local/e2e/.env on first run -- fresh clones get
working defaults with zero manual setup; internal port edits the
auto-created file once.

Drive-by repairs to unblock make lint (pre-existing failures from #161 / a
recent formatter rule, blocking any commit on this branch):
- history-service/internal/mongorepo/room.go: qualify mongoutil.Collection,
  mongoutil.NewCollection, mongoutil.WithProjection (sibling files already
  use the qualified form).
- history-service/internal/service/service.go: add the missing
  internal/mongorepo import that the file already references.
- pkg/minioutil/minio_integration_test.go: gofmt inline struct decls to
  multi-line per current linter config.
GITMateuszCharczuk pushed a commit that referenced this pull request May 8, 2026
Small Resource-Owner-Password-Credentials client at pkg/testutil/keycloak.
Used by the upcoming e2e harness to mint JWTs for test users, and reusable
from any future per-service integration test that needs Keycloak-backed
auth without rolling its own HTTP wrapper.

- Client.PasswordGrant / AccessToken / Refresh
- Sentinel errors (ErrInvalidCredentials, ErrUnknownClient, ErrRealmNotFound)
  with errors.Is wired via *Error.Is
- Built on pkg/restyutil so OTel + slog + project-default timeout are inherited
- 97% statement coverage; httptest-based unit tests, no external deps

Spec updated: helper extraction folded into the e2e plan, and the bootstrap
ergonomics now use a checked-in .env.example with a Makefile prerequisite
that auto-creates docker-local/e2e/.env on first run -- fresh clones get
working defaults with zero manual setup; internal port edits the
auto-created file once.

Drive-by repairs to unblock make lint (pre-existing failures from #161 / a
recent formatter rule, blocking any commit on this branch):
- history-service/internal/mongorepo/room.go: qualify mongoutil.Collection,
  mongoutil.NewCollection, mongoutil.WithProjection (sibling files already
  use the qualified form).
- history-service/internal/service/service.go: add the missing
  internal/mongorepo import that the file already references.
- pkg/minioutil/minio_integration_test.go: gofmt inline struct decls to
  multi-line per current linter config.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants